Skip to content

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Oct 20, 2025

What do these changes do?

Changes how the stop procedure works. It no longer involves long running http connections.
Stop returns immediately and the status of the service is monitor until it becomes idle, marking the removal of the service.
This approach works for both legacy and new style dynamic services.

The dynamic-scheduler now creates a fire_and_forget task to deal with stopping legacy services, since it is still required to wait to up to 1 hour for these to finish stopping.

Related issue/s

How to test

Dev-ops

@GitHK GitHK self-assigned this Oct 20, 2025
@GitHK GitHK added this to the Imparable milestone Oct 20, 2025
@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

❌ Patch coverage is 68.62745% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.33%. Comparing base (801bbde) to head (e341566).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8531      +/-   ##
==========================================
- Coverage   87.52%   87.33%   -0.20%     
==========================================
  Files        2010     1578     -432     
  Lines       78561    65665   -12896     
  Branches     1349      682     -667     
==========================================
- Hits        68762    57349   -11413     
+ Misses       9395     8075    -1320     
+ Partials      404      241     -163     
Flag Coverage Δ
integrationtests 63.94% <30.76%> (-0.02%) ⬇️
unittests 85.78% <68.62%> (-0.45%) ⬇️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 70.94% <ø> (ø)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 84.83% <ø> (-0.12%) ⬇️
agent 93.10% <ø> (ø)
api_server 91.62% <ø> (ø)
autoscaling 95.00% <ø> (ø)
catalog 92.06% <ø> (ø)
clusters_keeper 99.14% <ø> (ø)
dask_sidecar 92.38% <ø> (ø)
datcore_adapter 97.95% <ø> (ø)
director 75.72% <ø> (ø)
director_v2 90.91% <ø> (-0.02%) ⬇️
dynamic_scheduler 96.66% <81.57%> (-0.16%) ⬇️
dynamic_sidecar 90.44% <ø> (ø)
efs_guardian 89.83% <ø> (ø)
invitations 90.90% <ø> (ø)
payments 92.80% <ø> (ø)
resource_usage_tracker 92.22% <ø> (+0.05%) ⬆️
storage 86.56% <ø> (-0.29%) ⬇️
webclient ∅ <ø> (∅)
webserver 87.08% <30.76%> (+<0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 801bbde...e341566. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify
Copy link
Contributor

mergify bot commented Oct 20, 2025

🧪 CI Insights

Here's what we observed from your CI run for e341566.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI integration-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View
unit-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

@GitHK GitHK added the t:maintenance Some planned maintenance work label Oct 20, 2025
@GitHK GitHK added the bug buggy, it does not work as expected label Oct 21, 2025
@GitHK GitHK requested a review from Copilot October 21, 2025 08:54
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the dynamic service stop mechanism to use an asynchronous polling approach instead of long-running HTTP connections. The stop operation now returns immediately and monitors the service status until it reaches an idle state. For legacy services that may take up to 1 hour to stop, the dynamic-scheduler creates fire-and-forget tasks to handle the stopping process without blocking.

Key changes:

  • Webserver now polls service status after initiating stop instead of waiting on a long HTTP connection
  • Dynamic-scheduler uses fire-and-forget tasks for legacy service stops that may take extended time
  • Removed timeout parameter from RPC interface as stops are now non-blocking

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
services/web/server/src/simcore_service_webserver/dynamic_scheduler/api.py Implements polling mechanism to wait for service to become idle after stop request
services/dynamic-scheduler/tests/unit/api_rpc/test_api_rpc__services.py Removes timeout_s parameter from stop_dynamic_service test calls
services/dynamic-scheduler/src/simcore_service_dynamic_scheduler/services/fire_and_froget.py Adds new FireAndForgetCollection to manage background tasks
services/dynamic-scheduler/src/simcore_service_dynamic_scheduler/services/director_v2/_public_client.py Adds cleanup of public client from app state
services/dynamic-scheduler/src/simcore_service_dynamic_scheduler/services/common_interface.py Distinguishes legacy vs new-style services and uses fire-and-forget for legacy stops
services/dynamic-scheduler/src/simcore_service_dynamic_scheduler/services/catalog/_public_client.py Renames get_services_labels to get_service_labels
services/dynamic-scheduler/src/simcore_service_dynamic_scheduler/core/events.py Registers fire_and_forget_lifespan
services/dynamic-scheduler/src/simcore_service_dynamic_scheduler/api/frontend/routes_external_scheduler/_service.py Removes unused timeout_s parameter from service_stop
services/director-v2/src/simcore_service_director_v2/core/dynamic_services_settings/scheduler.py Updates comment to clarify timeout applies to LEGACY services
services/director-v2/src/simcore_service_director_v2/api/routes/dynamic_services.py Removes polling logic from stop endpoint as it's now handled by caller
packages/service-library/src/servicelib/rabbitmq/rpc_interfaces/dynamic_scheduler/services.py Removes timeout_s parameter from stop_dynamic_service RPC interface

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Andrei Neagu added 2 commits October 21, 2025 10:58
@GitHK GitHK requested a review from Copilot October 21, 2025 09:05
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@GitHK GitHK changed the title 🎨 Changed how service stop works 🎨 No more long running http requests while stopping services Oct 21, 2025
@GitHK GitHK marked this pull request as ready for review October 21, 2025 09:36
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx. Left some comments.

I recommend you to cleanup naming typos, scheck suggestions and I would also make sure has a good test coverage

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@GitHK GitHK enabled auto-merge (squash) October 22, 2025 07:18
@GitHK GitHK added the 🤖-automerge marks PR as ready to be merged for Mergify label Oct 22, 2025
@GitHK
Copy link
Contributor Author

GitHK commented Oct 22, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2025

queue

🛑 Configuration not compatible with a branch protection setting

The branch protection setting Require branches to be up to date before merging is not compatible with draft PR checks. To keep this branch protection enabled, update your Mergify configuration to enable in-place checks: set merge_queue.max_parallel_checks: 1, set every queue rule batch_size: 1, and avoid two-step CI (make merge_conditions identical to queue_conditions). Otherwise, disable this branch protection.

@GitHK GitHK disabled auto-merge October 22, 2025 10:41
@GitHK GitHK enabled auto-merge (squash) October 22, 2025 10:41
@sonarqubecloud
Copy link

@GitHK GitHK merged commit 098f490 into ITISFoundation:master Oct 22, 2025
93 of 95 checks passed
@GitHK GitHK deleted the pr-osparc-rework-shutdown branch October 22, 2025 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify bug buggy, it does not work as expected t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants